-
Notifications
You must be signed in to change notification settings - Fork 3
Add method to save Cadet sim as python file which can generate the Cadet sim again #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
e06708a to
efefceb
Compare
efefceb to
b761ac0
Compare
|
This has been rebased and the tests still run. What was the reason this wasn't merged again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there...
I added docstrings and type annotations.
Moreover, I also refactored the method s.t. it not only works for Cadet objects but also more generically also for H5 types (where the method lives) or other derived classes.
|
I just talked to @jbreue16. He would like to test this with a simulation file that also contains output. 🤓 Maybe we need some check on the size of the parameters or we should add an argument to specify / limit the branches to be "imported". Other than that, looks really good. Thanks for pushing it; I had already forgotten about it. 😅 |
|
ref_2DGRM3Zone_dynLin_1Comp_radZ3.zip Writing the attached h5 with the output data to a python file using the added functionality, I observed three issues:
Update: deleting one line seems to fix the second point. why was this added to the code anyways? does soemthing else break or is this required for array? UpdateUpdate: seems to be required somewhere else, needs a different fix |
|
Thanks for the additional test-case! It helped catch two extra cases that I've been able to fix two of.
Is fixed by 1416997 Additional cases from your example: On my machine it threw an error for regarding:
I don't observe that. Is this still present in the current version of the PR for you? Regarding:
That is because numpy changed it's |
|
I can't seem to re-run the CI pipeline. I'll close this PR and re-open one to trigger the CI. |
|
Re-opening the PR helped to run the CI. All tests pass. |
|
Regarding the last open problem:
That was solved with the change in 1416997 by the switch from |
|
To summarize: I've fixed all problems that arose from Jan's test case and extended the CI test to always include these cases. I've changed the CI to use the create_LWE to create and run a simulation that we thereafter serialize and de-serialize so that we're always working with actual simulation data in our tests. Docstrings have been added. I think it's ready to be squased and merged. |
|
Looks good to me, thanks Ron ! |
Also add test for the save_as_python method
Also changed a few lines in cadet.py to follow PEP 8